[ECO-5118] Added tests for presence and occupancy #197
Conversation
maratal
force-pushed
the
114-tests
branch
3 times, most recently
from
a58094a to
7539cf7
Compare
maratal
changed the title
github-actions
bot
temporarily deployed
to
staging/pull/197/AblyChat
github-actions
bot
temporarily deployed
to
staging/pull/197/AblyChat
github-actions
bot
temporarily deployed
to
staging/pull/197/AblyChat
WalkthroughThis pull request introduces several enhancements to the AblyChat framework's testing and presence management infrastructure. The changes include adding a new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/197/AblyChat
github-actions
bot
temporarily deployed
to
staging/pull/197/AblyChat
maratal
changed the title
github-actions
bot
temporarily deployed
to
staging/pull/197/AblyChat
github-actions
bot
temporarily deployed
to
staging/pull/197/AblyChat
github-actions
bot
temporarily deployed
to
staging/pull/197/AblyChat
maratal
force-pushed
the
114-tests
branch
2 times, most recently
from
6fa21e5 to
6c63f61
Compare
github-actions
bot
temporarily deployed
to
staging/pull/197/AblyChat
github-actions
bot
temporarily deployed
to
staging/pull/197/AblyChat
|
Spec coverage test issue - ably/specification#269 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (1)
36-54: Enhance subscription test coverage.While the test verifies basic subscription functionality, consider testing multiple events and edge cases:
- Multiple occupancy updates
- Zero occupancy
- Maximum connection/presence counts
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (1)
93-96: Consider adding bounds checking for the delay parameter.While the implementation is correct, it might be good to add an upper bound check to prevent excessive delays in tests.
case let .completeAndChangeState(completeResult, newState, milliseconds): + guard milliseconds <= 5000 else { + throw ARTErrorInfo(message: "Delay cannot exceed 5 seconds") + } if milliseconds > 0 { try? await Task.sleep(nanoseconds: milliseconds * 1_000_000) }Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (1)
89-95: Consider using a more robust subscription management system.The current implementation only supports a single subscriber. Consider using a collection to support multiple subscribers.
- private var subscribeCallback: ARTPresenceMessageCallback? + private var subscribers: [(ARTEventListener, ARTPresenceMessageCallback)] = [] func subscribe(_ callback: @escaping ARTPresenceMessageCallback) -> ARTEventListener? { - subscribeCallback = callback + let listener = ARTEventListener() + subscribers.append((listener, callback)) for member in members { - subscribeCallback?(member) + callback(member) } - return ARTEventListener() + return listener }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Sources/AblyChat/Dependencies.swift(1 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(6 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomPresenceTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(5 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: SPM (Xcode 16)
- GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode, macOS (Xcode 16)
🔇 Additional comments (22)
Sources/AblyChat/Dependencies.swift (1)
28-28: LGTM!The new
RealtimePresenceProtocolis well-defined with appropriate protocol inheritance. TheSendableconformance is particularly good for actor isolation.Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (2)
8-31: LGTM! Well-structured occupancy check test.The test effectively validates the occupancy information retrieval with good mock setup and clear assertions.
58-74: LGTM! Good discontinuity event test.The test effectively validates discontinuity event propagation with appropriate error simulation.
Tests/AblyChatTests/Helpers/Helpers.swift (3)
25-32: LGTM! Clean convenience initializer.The convenience initializer for
ARTPresenceMessageis well-designed with appropriate defaults.
34-41: LGTM! Comprehensive presence event types.The static array provides a complete list of presence event types, which is useful for testing all presence scenarios.
43-82: LGTM! Well-structured test helpers.The
RoomLifecycleHelperprovides comprehensive utilities for test setup with:
- Clear documentation for the network delay constant
- Well-named parameters with sensible defaults
- Flexible configuration options for different test scenarios
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (1)
48-48: LGTM! Good addition of network delay simulation.The optional delay parameter with a default value of 0 is a good design choice for backward compatibility.
Sources/AblyChat/RoomFeature.swift (1)
66-66: LGTM! Good use of protocol composition.The change to use protocol composition with
any RoomLifecycleContributor & EmitsDiscontinuitiesimproves flexibility while maintaining type safety.Tests/AblyChatTests/DefaultMessagesTests.swift (1)
13-13: LGTM! Good improvement in test setup clarity.The consistent addition of
attachResult: .successmakes the channel attachment state explicit in test setup, improving test clarity and maintainability.Also applies to: 31-31, 55-56, 84-84, 119-119, 148-148
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (4)
8-12: LGTM! Clean implementation of presence-related properties.The mock presence implementation is well-structured, with a private property for the mock and a public computed property that exposes it through the protocol interface.
34-43: LGTM! Proper initialization of mock presence.The initializer correctly handles the optional mock presence parameter with a default value of nil, maintaining backward compatibility.
75-75: LGTM! State property now reflects attachment status.The state property implementation now correctly reflects the channel's attachment status, returning
.attachedfor successful attachments and.failedfor failures.
Line range hint
90-187: LGTM! Enhanced enum and event listener implementation.Good improvements:
- AttachOrDetachResult now conforms to Equatable, enabling equality comparisons in tests
- The
onmethod now returns a valid ARTEventListener instead of fatalErrorTests/AblyChatTests/DefaultRoomPresenceTests.swift (9)
1-5: LGTM! Proper test file setup with necessary imports.The test file is well-organized with appropriate imports for testing and the target module.
8-20: LGTM! Clear channel name validation test.The test properly validates that the channel name follows the expected format:
{roomId}::$chat::$chatMessages.
27-118: LGTM! Comprehensive presence enter tests.The tests thoroughly cover presence enter scenarios:
- Basic enter operation
- Enter while attaching
- Enter with attachment failure
- Error handling for invalid room states
148-261: LGTM! Thorough presence update tests.The tests comprehensively cover presence update scenarios:
- Basic update operation
- Update while attaching
- Update with attachment failure
- Error handling for invalid room states
267-280: LGTM! Clear presence leave test.The test properly validates that a user can leave presence and verifies the empty presence set afterward.
286-300: LGTM! User presence check test.The test effectively verifies both positive and negative cases for user presence checks.
307-416: LGTM! Comprehensive presence set retrieval tests.The tests thoroughly cover get operations:
- Basic retrieval
- Retrieval while attaching
- Retrieval with attachment failure
- Error handling for invalid states
423-464: LGTM! Thorough presence events subscription test.The test effectively validates subscription to all presence events:
- present
- enter
- update
- leave
470-485: LGTM! Clear discontinuity handling test.The test properly validates that discontinuity events are propagated through the presence implementation.
Closes #114
Summary by CodeRabbit
Release Notes
New Features
Tests
Improvements